ext/opcache/jit/zend_jit_trace.c: In function ‘zend_jit_trace’:
ext/opcache/jit/zend_jit_trace.c:6383:23: error: ‘gen_handler’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

This CI failure is not-quite-related to this commit. Apparently code refactoring has made this GCC warning suddenly reachable, but I don't see an obvious reason why. The general structure of this piece of code wasn't changed. The code was questionable, and still is - I don't understand it - @dstogov, can you explain this obscure variable and the strange ways it's being assigned?

moving declarations and adding const everywhere doesn't improve anything

Uh-oh, this is a big red flag - it sounds like we are in disagreement over fundamental coding things. I guess it's better if I stop trying to improve this piece of code before I waste more time, which is sad because it would have been the path to fix all those remaining PHP JIT crash bugs we have been experiencing, which forced us to disable the JIT completely.

On the other hand, if the old JIT is frozen until it gets replaced with the new one, we can easily keep our improvements (and crash bug fixes) in our private PHP fork without fearing merge conflicts, but it won't be possible to contribute them back to this project.

moving declarations and adding const everywhere doesn't improve anything

Uh-oh, this is a big red flag - it sounds like we are in disagreement over fundamental coding things.

No complete disagreement, just priorities.

I guess it's better if I stop trying to improve this piece of code before I waste more time, which is sad because it would have been the path to fix all those remaining PHP JIT crash bugs we have been experiencing, which forced us to disable the JIT completely.

Changing code style won't fix JIT problems and re-factoring may easily add new ones.
And yes, it's not the best time for re-factoring.

On the other hand, if the old JIT is frozen until it gets replaced with the new one,

Yeah, I stopped the future development of the existing JIT, doing only the necessary fixes.
My development branch contains both - the existing and new experimental IR based JIT (it may be enabled during configure). After merging and switching to new JIT, the existing JIT implementation should be removed.
There is no reason to re-factor its code.

The new JIT branch is in development stage, and the code is even more dirty yet (because I have to keep the old JIT and minimize the diff)

we can easily keep our improvements (and crash bug fixes) in our private PHP fork without fearing merge conflicts, but it won't be possible to contribute them back to this project.

Bug fixes are usually unrelated to re-factoring, especial if we have to back-port them to released PHP branches anyway.

Sorry, at this moment, I may only cherry-pick some minor commits.
I would propose to postpone the code style changes.

Changing code style won't fix JIT problems

That is technically correct, but only technically, which makes this kind of a fallacy.

The reality is that the JIT is a huge pile of obscure code that nobody but you understands. Of course, a JIT is always complex, but your special coding style (for example using goto everywhere, even for jumping between if/else branches and for simulating loops; or huge functions with thousands of lines) makes this more difficult than necessary.

Simplifying the code would make the code understandable by others. Understanding your code is necessary for anybody to fix a bug. Having more than one guy on this planet being able to fix JIT bugs fixes more JIT bugs.

It took me a whole week to come up with #10138 and #10153 - a week of insomnia because those bugs haunted me day and night. My fixes are not robust, I don't trust them - and I have a feeling the whole JIT is not very robust; not just race conditions, but also due to having redundant copies of huge sections of the PHP interpreter.

There are more JIT bugs I can reproduce easily. With code refactoring, I'd get a grip on those sooner or later; but without code refactoring, I rather won't touch the JIT ever again, and keep it disabled permanently. And hope the new JIT is more robust than the current one. (Oh no, the goto-meter says there are 190 gotos in the ir repository alone!)

I'm too old to argue about code style. Most of PHP the code base is written in the same style and I didn't see a reason to change this. I know, my code is not ideal, but this is mostly because of the huge tasks and practical priorities. If you like to make the JIT code to look more modern, you are welcome. Just not at this moment.

And hope the new JIT is more robust than the current one. (Oh no, the goto-meter says there are 190 gotos in the ir repository alone!)

You probably counted goto(s) in the the auto-generated parser... Anyway, I don't think goto is always terribly bad. I use it for code de-duplication and exits from inner-loops.

my code is not ideal, but this is mostly because of the huge tasks and practical priorities

I can relate to that, that's what I'd say about every piece of code I wrote. I'm not here to bash your code, I'm here to be constructive and help with fixing it. The fact that this JIT exists is your big achievement, but it's not ready for production due to its many remaining crash bugs (looking at #7817).

There are lots of different tastes about coding style. Tabs vs spaces makes no quality difference at all, for example. But there are coding style aspects which people can disagree on, but which do have a measurable practical effect on quality and stability, like always using const and not reusing variables for different purposes (less obscurity and more compiler insight to show warnings), keeping functions small (nobody can understand a multi-thousand-line monster) and not using goto (obscures code paths). All of these aspects are being addressed by my work.

Look at this gen_handler variable (#10206 (comment)) - a compiler warning that was revealed by code refactoring; I'm almost convinced that this is indeed a bug in your code. In any case, no matter how hard I try, I cannot explain why the variable is (not) initialized that way. With proper coding style, this bug could have been prevented easily. I'm sure my work would have revealed many more similar bugs.

About goto - sure, some people who write plain C like to reduce duplicate code in error paths by using goto, and it's sometimes hard to resist the temptation (though I'd prefer not to use plain C at all, because only C burdens you with manual error handling and resource management). But the way goto is being used in the JIT is sometimes not that, and I think it's being used terribly badly, but that's just my taste. Certainly the use of goto obscures the code for no reason, and I have demonstrated that it can be done without it, see commit 4dd445c in this draft PR.

Being "too old" or writing "not ideal code" is not a big deal (both applies to me & my code just as well) - resistance to improve is the problem. The fact that you're writing a new JIT shouldn't be an excuse for blocking improvements (done by others) on the old one, and fearing merge conflicts with that JIT rewrite is an even more awful excuse.

I can relate to that, that's what I'd say about every piece of code I wrote. I'm not here to bash your code, I'm here to be constructive and help with fixing it.

I see.

The fact that this JIT exists is your big achievement, but it's not ready for production due to its many remaining crash bugs (looking at #7817).

Right. JIT still has bugs and often almost useless for real-life apps. It should be enabled/disabled after testing of your app(s). Make it more mature is possible by catching and fixing the problems. Unfortunately, often people can't provide a way to reproduce the problems they see.

The fact that you're writing a new JIT shouldn't be an excuse for blocking improvements (done by others) on the old one, and fearing merge conflicts with that JIT rewrite is an even more awful excuse.

Changing code style is not an improvement and we don't need it by itself. It may make sense to do it together with some major changes. Otherwise this would make troubles with every bug fix. And yes. This will complicate my development, as I did all the best to keep the diff minimal.

ext/opcache/jit/zend_jit_trace.c: In function ‘zend_jit_trace’:
ext/opcache/jit/zend_jit_trace.c:6383:23: error: ‘gen_handler’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

This CI failure is not-quite-related to this commit. Apparently code refactoring has made this GCC warning suddenly reachable, but I don't see an obvious reason why. The general structure of this piece of code wasn't changed. The code was questionable, and still is - I don't understand it - @dstogov, can you explain this obscure variable and the strange ways it's being assigned?

I checked this, it's a false compiler warning: it cannot be used uninitialized. gen_handler is only used if ssa_op is non-NULL. ssa_op is non-NULL if JIT_G(opt_level) >= ZEND_JIT_LEVEL_INLINE. But if JIT_G(opt_level) >= ZEND_JIT_LEVEL_INLINE is true, then gen_handler will definitely have a value: the very first statement of the if body sets it to zero.

To be extra sure, I further validated this with dynamic testing: setting gen_handler to a sentinel value and then adding a ZEND_ASSERT(gen_handler != sentinel);, it didn't trigger running the test suite. Combine that with my reasoning above I'm convinced this luckily isn't a bug :)

I checked this, it's a false compiler warning

Thanks @nielsdos, that sounds right. I was confused by those 2000 lines of obscure code between the declaration of the variable and the usage of the variable, and several changes of the ssa_op variable in between, but those are irrelevant for whether get_handler is initialized.

The new JIT branch is in development stage

@dstogov what would be a very approximate time frame for the possible release of the new JIT ? weeks, months, years ?

I hope, it's going to be merged into master in this year.